Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Testing: Add e2e test for basic ContrastChecker functionality #68856

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

himanshupathak95
Copy link
Contributor

@himanshupathak95 himanshupathak95 commented Jan 23, 2025

Closes: #68853

What?

Add a basic end-to-end test for the ContrastChecker component that verifies the low-contrast warning appears immediately when the text and background colors have insufficient contrast.

Why?

After enhancing the ContrastChecker component in #68799, it's important to ensure its core functionality of detecting and warning about low-contrast color combinations works reliably.

Testing Instructions

  • Run npm run test:e2e -- editor/various/contrast-checker.spec.js to execute the test
  • Verify that it is passing and debug (if necessary) to check the working

Screencast

Screen.Recording.2025-01-24.at.13.25.25.mov

@himanshupathak95 himanshupathak95 marked this pull request as ready for review January 24, 2025 07:57
Copy link

github-actions bot commented Jan 24, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: himanshupathak95 <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: afercia <[email protected]>
Co-authored-by: juanfra <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@juanfra juanfra added [Type] Bug An existing feature does not function as intended [Feature] Colors Color management labels Jan 24, 2025
@Mamaduka Mamaduka self-requested a review January 27, 2025 08:05
@afercia
Copy link
Contributor

afercia commented Jan 27, 2025

After the first test gets refined, it would be nice to add another test as @juanfra mentioned in #68853 (comment)
To my understanding, it would be basically: set only the text color to white and see if the contrastChecker warns against the default background color.

} );

await page.getByRole( 'button', { name: 'Text', exact: true } ).click();
await page.getByLabel( 'Black' ).click();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be located using the following getByRole selector.

await page.getByRole( 'option', { name: 'Black' } ).click();

await page.getByLabel( 'Black' ).click();

await page.getByRole( 'button', { name: 'Background' } ).click();
await page.getByLabel( 'Black' ).click();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

@Mamaduka
Copy link
Member

@himanshupathak95, thanks for refactoring. Left two minor notes. Let's add the test case @afercia mentioned and this should be good to merge.

I think we can just use test steps and test everything in a single test.

@himanshupathak95 himanshupathak95 force-pushed the add/e2e-test-contrast-checker branch from d20c61f to 2635c70 Compare January 30, 2025 08:24
@himanshupathak95
Copy link
Contributor Author

himanshupathak95 commented Jan 31, 2025

Hey @Mamaduka, The failing test is passing locally for me -

Screen.Recording.2025-01-31.at.12.09.35.mov

image

While I'm debugging this, could you please confirm if the tests are passing on your end whenever it's convenient for you?

@Mamaduka
Copy link
Member

Thanks, @himanshupathak95!

I'll try to have a look at e2e tests next week.

@afercia
Copy link
Contributor

afercia commented Jan 31, 2025

The Check white text on default background step fails for me locally. The screenshot from the test results artifacts suggests there is a problem with the contrast checker in the first place as it doesn't show the notice:

test-failed-1

I tested again the Contrast checker and it seems that right after adding some text in a paragraph and setting the text color the first time, the contrast checker doesn't appear indeed. Animated GIF:

contrast

In a way, I guess this test failure surfaces a potential bug in the Contrast checker that should be double checked first.

@himanshupathak95
Copy link
Contributor Author

himanshupathak95 commented Jan 31, 2025

Thanks for testing this @afercia. You're correct that the warning appears, but not always as it should -

Screen.Recording.2025-01-31.at.15.03.24.mov

@afercia
Copy link
Contributor

afercia commented Jan 31, 2025

Hm, for me, when running the Gutenberg wp-env, Twenty Twenty-One shows the Default color palette. It should show its own color palette. Not sure why.
Anyways, I can't reproduce the bug consistently but it seems to happen only with the Gutenberg wp-env and the Default palette.
I can't reproduce it when running the WordPress wp-env with Gutenberg installed as a plugin, and in this case Twenty Twenty-One shows the Theme color palette.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add basic e2e test for the most ContrastChecker simple cases
4 participants